Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dockerized Commands in the packaging spec #129

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

daniel-chambers
Copy link
Collaborator

@daniel-chambers daniel-chambers commented Mar 22, 2024

@daniel-chambers daniel-chambers self-assigned this Mar 22, 2024
Comment on lines +67 to +68
update?: string | DockerizedCommand
watch?: string | DockerizedCommand
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to allow a string array for args in the Docker command, let's do it for native commands too.

Suggested change
update?: string | DockerizedCommand
watch?: string | DockerizedCommand
update?: string | string[] | DockerizedCommand
watch?: string | string[] | DockerizedCommand

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure about that. For native commands, that is not "args", it's a shell command, so it's one string because it ends up as one arg. It's what get passed to (for example) bash -c.

bash -c "echo \"123\"" is not the same as bash -c "echo" "123".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I want the capability to provide commands + args directly, not just a shell command. There's no reason to invoke a shell for echo 123, after all, if we can represent it as ["echo", "123"].

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. @SandeepSamba do you have the capacity to implement a third methodology here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it to be just a single string command rather than string[]. We have to invoke the commands via a shell because without a shell, all the fancy(coloured output, formatting,etc) logging that the plugins provide are lost. Even if we provide a string[], the CLI wil concat them with spaces and call it with shell. And with the native packaging RFC we will anyway have to bring in a shell. @SamirTalwar Do you have any particular issue due to the choice of single string vs string[]?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SandeepSamba the problem is which shell? Isn't that the point of this. To not use a shell, and just have the CLI run the docker command. Bash isn't normally or always available on windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codedmart I think @SandeepSamba's point is that he doesn't want to implement string[] (which bypasses the shell) as an alternative to string (which uses the shell), because bypassing the shell stuffs up formatting. The DockerizedCommand is separate to string and string[]. @SamirTalwar's suggestion of adding string[] was in addition to DockerizedCommand.

@nullxone
Copy link
Contributor

nullxone commented Apr 9, 2024

@SandeepSamba is this already implemented?

@SandeepSamba
Copy link

@SandeepSamba is this already implemented?

No, this will have to to be done once the RFC is approved

@daniel-chambers daniel-chambers merged commit ab53769 into main Apr 10, 2024
8 checks passed
@daniel-chambers daniel-chambers deleted the daniel/dockerized-commands-rfc branch April 10, 2024 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants